Fix persistent comments for Acknowledgements #4956

TheFlyingCorpse · 2017-01-25T20:26:04Z

TheFlyingCorpse · 2017-01-25T20:33:14Z

Note: I used the same way of setting the bool persistent / false as commented earlier as is already used for the ApiAction::ScheduleDowntime command.

dnsmichi · 2017-01-26T08:34:00Z

lib/icinga/apiactions.cpp

@@ -200,7 +200,7 @@ Dictionary::Ptr ApiActions::AcknowledgeProblem(const ConfigObject::Ptr& object,
 	if (params->Contains("sticky"))
 		sticky = AcknowledgementSticky;


That should check for a boolean, not just for the attribute being set.

Similar fix to be applied: 8196523

if (params->Contains("sticky") && HttpUtility::GetLastParameter(params, "sticky"))

Updated to convert the number to bool properly.

gunnarbeutner · 2017-02-08T07:38:08Z

Can you remove the unnecessary explicit '== true', '> 0', etc. checks?

Also, aren't all non-persistent comments in Icinga 1.x removed on restart (by virtue of not being persisted at all)? Your patch seems to not remove expired comments at all if they're persistent. Is that the correct behavior?

dnsmichi · 2017-02-10T08:10:51Z

The naming with persistent just for acknowledgement comments is somewhat ambiguous. For "normal" comments there's no exact meaning, you cannot change or tell anything. Comments of the type Acknowledgement will only require an additional state to determine whether the comment is being deleted upon ack removal or not.

TheFlyingCorpse · 2017-02-10T08:23:17Z

Right on the updated documentation, I see it now when you point out the ambiguity. So remove the document update and squash commits? Anything else?

…

Den 10. feb. 2017 kl. 09.10 skrev Michael Friedrich ***@***.***>: The naming with persistent just for acknowledgement comments is somewhat ambiguous. For "normal" comments there's no exact meaning, you cannot change or tell anything. Comments of the type Acknowledgement will only require an additional state to determine whether the comment is being deleted upon ack removal or not. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or mute the thread.

dnsmichi · 2017-02-10T09:28:48Z

Squashing would certainly help an additional review, not sure about the docs. I would opt for finding a better name for persistent. That is for the changes inside the .ti file as well as the function parameters inside the source code.

TheFlyingCorpse · 2017-02-14T21:55:14Z

Updated, let me know if I didnt do it correctly. :-)

TheFlyingCorpse · 2017-03-01T10:48:29Z

Rebased based on master, there was a conflict with doc and apiactions after an update to notify.

TheFlyingCorpse · 2017-03-21T21:24:50Z

Feedback from @dnsmichi is that he does not like the proposed name of comment_persistance.

Other suggestions:

comment_from_ack_persistent
acknowledgement_persistance
acknowledgement_expiry

I'd really like to see this in 2.7 as the bug causes us a lot of headaches when comments from acknowledgements disappear with ticket numbers that was the comment for the ack.

fixes Icinga#4818 Signed-off-by: Michael Friedrich <michael.friedrich@icinga.com>

dnsmichi · 2017-05-10T15:13:06Z

Review

I don't have a better name. I'd go with the same name as known from 1.x and external commands, just persistent. In contrast to the old world, our comments are never removed upon daemon restart (that is really nonsense).

The docs should be clear about that, and if one tends to fetch the values from i.e. the API, he/she needs to be sure to check entry_type == 4 && persistent == ? then.

In terms of upgrade safety, one would say, persistent should default to false. All comments inside the _api package won't have that flag set, and would likely to continue being removed on host recovery.
On the other hand queries will look strange with persistent returning false. But that is purely cosmetic and can only be explained in the docs.

I wouldn't change that behaviour and leave your patch as is.

Patch

I've modified some code parts following our code style, and removed a duplicate check. Rebased against master and amended my changes into your commit. If've also taken the liberty of force pushing your remote branch to cleanly merge this PR.

Sorry for keeping this so long on my TODO list. I wanted to make sure I've fully understand your concern and proposed patch. The description with ticket ids in comments totally make sense to me now.

Thanks for the patch 👍

Tests

Non-persistent

Add

curl -k -s -u root:icinga -H 'Accept: application/json' -X POST 'https://localhost:5665/v1/actions/acknowledge-problem' -d '{ "author": "michi", "comment": "jo eh.", "type": "Service", "filter": "match(\"mbmif*disk /\", service.__name) && service.state != ServiceOK" }'

curl -k -s -u root:icinga -H 'Accept: application/json' -H 'X-HTTP-Method-Override: GET' -X POST 'https://localhost:5665/v1/objects/comments' -d '{ "joins": [ "service.__name", "service.acknowledgement" ], "filter": "comment.entry_type == 4", "attrs": [ "__name", "author", "persistent", "text" ] }' | python -m json.tool
{
    "results": [
        {
            "attrs": {
                "__name": "mbmif.int.netways.de!disk /!mbmif.int.netways.de-1494428256-9",
                "author": "michi",
                "persistent": false,
                "text": "jo eh."
            },
            "joins": {
                "service": {
                    "__name": "mbmif.int.netways.de!disk /",
                    "acknowledgement": 1.0
                }
            },
            "meta": {},
            "name": "mbmif.int.netways.de!disk /!mbmif.int.netways.de-1494428256-9",
            "type": "Comment"
        }
    ]
}

Remove

curl -k -s -u root:icinga -H 'Accept: application/json' -X POST 'https://localhost:5665/v1/actions/remove-acknowledgement' -d '{ "type": "Service", "filter": "match(\"mbmif*disk /\", service.__name) && service.state != ServiceOK" }'

$ curl -k -s -u root:icinga -H 'Accept: application/json' -H 'X-HTTP-Method-Override: GET' -X POST 'https://localhost:5665/v1/objects/comments' -d '{ "joins": [ "service.__name", "service.acknowledgement" ], "filter": "comment.entry_type == 4", "attrs": [ "__name", "author", "persistent", "text" ] }' | python -m json.tool
{
    "results": []
}

Persistent

Add

curl -k -s -u root:icinga -H 'Accept: application/json' -X POST 'https://localhost:5665/v1/actions/acknowledge-problem' -d '{ "author": "michi", "comment": "jo eh.", "persistent": true, "type": "Service", "filter": "match(\"mbmif*disk /\", service.__name) && service.state != ServiceOK" }

curl -k -s -u root:icinga -H 'Accept: application/json' -H 'X-HTTP-Method-Override: GET' -X POST 'https://localhost:5665/v1/objects/comments' -d '{ "joins": [ "service.__name", "service.acknowledgement" ], "filter": "comment.entry_type == 4", "attrs": [ "__name", "author", "persistent", "text" ] }' | python -m json.tool
{
    "results": [
        {
            "attrs": {
                "__name": "mbmif.int.netways.de!disk /!mbmif.int.netways.de-1494428328-10",
                "author": "michi",
                "persistent": true,
                "text": "jo eh."
            },
            "joins": {
                "service": {
                    "__name": "mbmif.int.netways.de!disk /",
                    "acknowledgement": 1.0
                }
            },
            "meta": {},
            "name": "mbmif.int.netways.de!disk /!mbmif.int.netways.de-1494428328-10",
            "type": "Comment"
        }
    ]
}

Remove

curl -k -s -u root:icinga -H 'Accept: application/json' -X POST 'https://localhost:5665/v1/actions/remove-acknowledgement' -d '{ "type": "Service", "filter": "match(\"mbmif*disk /\", service.__name) && service.state != ServiceOK" }'

curl -k -s -u root:icinga -H 'Accept: application/json' -H 'X-HTTP-Method-Override: GET' -X POST 'https://localhost:5665/v1/objects/comments' -d '{ "joins": [ "service.__name", "service.acknowledgement" ], "filter": "comment.entry_type == 4", "attrs": [ "__name", "author", "persistent", "text" ] }' | python -m json.tool
{
    "results": [
        {
            "attrs": {
                "__name": "mbmif.int.netways.de!disk /!mbmif.int.netways.de-1494428328-10",
                "author": "michi",
                "persistent": true,
                "text": "jo eh."
            },
            "joins": {
                "service": {
                    "__name": "mbmif.int.netways.de!disk /",
                    "acknowledgement": 0.0
                }
            },
            "meta": {},
            "name": "mbmif.int.netways.de!disk /!mbmif.int.netways.de-1494428328-10",
            "type": "Comment"
        }
    ]
}

All fine ❤️

Icinga Web 2

Keep in mind that Icinga Web 2 doesn't send the persistent flag when the API command transport is used - that needs to be adopted too once this patch is merged and released. The API doesn't accept the flag in <= 2.6, while the external command listener silently ignores the passed value (and it doesn't return an error to the caller anyways).

I haven't tested the patch, but it should work. Maybe you can test that and send in a PR for Icinga Web 2 then :)

mbmif /usr/local/share/icingaweb2 (master *=) # git diff
diff --git a/modules/monitoring/library/Monitoring/Command/Renderer/IcingaApiCommandRenderer.php b/modules/monitoring/library/Monitoring/Command/Renderer/IcingaApiCommandRenderer.php
index 30dc414dc..edc5946ed 100644
--- a/modules/monitoring/library/Monitoring/Command/Renderer/IcingaApiCommandRenderer.php
+++ b/modules/monitoring/library/Monitoring/Command/Renderer/IcingaApiCommandRenderer.php
@@ -181,7 +181,8 @@ class IcingaApiCommandRenderer implements IcingaCommandRendererInterface
             'comment'       => $command->getComment(),
             'expiry'        => $command->getExpireTime(),
             'sticky'        => $command->getSticky(),
-            'notify'        => $command->getNotify()
+            'notify'        => $command->getNotify(),
+            'persistent'    => $command->getPersistent()
         );
         $this->applyFilter($data, $command->getObject());
         return IcingaApiCommand::create($endpoint, $data);

dnsmichi · 2017-05-10T15:14:45Z

@lippserd see my note about the Icinga Web 2 API command transport :)

dnsmichi reviewed Jan 26, 2017

View reviewed changes

TheFlyingCorpse force-pushed the fix/persistent-comments-are-not-persistent branch from 56a7e93 to 848de5c Compare January 26, 2017 21:05

dnsmichi added area/db-ido Database output libicinga labels Feb 2, 2017

gunnarbeutner removed the libicinga label Feb 7, 2017

gunnarbeutner assigned TheFlyingCorpse Feb 8, 2017

TheFlyingCorpse force-pushed the fix/persistent-comments-are-not-persistent branch from 848de5c to 6a7f677 Compare February 9, 2017 17:59

TheFlyingCorpse added a commit to TheFlyingCorpse/icinga2 that referenced this pull request Feb 9, 2017

Update based on the feedback in Icinga#4956 and Icinga#4818

6a7f677

dnsmichi added the bug Something isn't working label Feb 10, 2017

TheFlyingCorpse force-pushed the fix/persistent-comments-are-not-persistent branch 2 times, most recently from a1754ba to 1f55cd9 Compare February 11, 2017 10:51

TheFlyingCorpse force-pushed the fix/persistent-comments-are-not-persistent branch 2 times, most recently from e8c9a44 to e492539 Compare March 1, 2017 10:47

TheFlyingCorpse force-pushed the fix/persistent-comments-are-not-persistent branch 4 times, most recently from 1604657 to 99cea7b Compare March 1, 2017 10:56

dnsmichi self-assigned this Apr 18, 2017

Crunsher force-pushed the master branch from 90a16d6 to 7c70d51 Compare May 2, 2017 08:55

Fix persistent comments for Acknowledgements

273ca6a

fixes Icinga#4818 Signed-off-by: Michael Friedrich <michael.friedrich@icinga.com>

dnsmichi force-pushed the fix/persistent-comments-are-not-persistent branch from 99cea7b to 273ca6a Compare May 10, 2017 15:11

dnsmichi merged commit 2196c6e into Icinga:master May 10, 2017

dnsmichi added this to the 2.7.0 milestone May 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix persistent comments for Acknowledgements #4956

Fix persistent comments for Acknowledgements #4956

TheFlyingCorpse commented Jan 25, 2017

TheFlyingCorpse commented Jan 25, 2017

dnsmichi Jan 26, 2017

TheFlyingCorpse Jan 26, 2017

gunnarbeutner commented Feb 8, 2017 •

edited

Loading

dnsmichi commented Feb 10, 2017

TheFlyingCorpse commented Feb 10, 2017 via email

dnsmichi commented Feb 10, 2017

TheFlyingCorpse commented Feb 14, 2017

TheFlyingCorpse commented Mar 1, 2017

TheFlyingCorpse commented Mar 21, 2017

dnsmichi commented May 10, 2017

dnsmichi commented May 10, 2017

		@@ -200,7 +200,7 @@ Dictionary::Ptr ApiActions::AcknowledgeProblem(const ConfigObject::Ptr& object,
		if (params->Contains("sticky"))
		sticky = AcknowledgementSticky;

Fix persistent comments for Acknowledgements #4956

Fix persistent comments for Acknowledgements #4956

Conversation

TheFlyingCorpse commented Jan 25, 2017

TheFlyingCorpse commented Jan 25, 2017

dnsmichi Jan 26, 2017

Choose a reason for hiding this comment

TheFlyingCorpse Jan 26, 2017

Choose a reason for hiding this comment

gunnarbeutner commented Feb 8, 2017 • edited Loading

dnsmichi commented Feb 10, 2017

TheFlyingCorpse commented Feb 10, 2017 via email

dnsmichi commented Feb 10, 2017

TheFlyingCorpse commented Feb 14, 2017

TheFlyingCorpse commented Mar 1, 2017

TheFlyingCorpse commented Mar 21, 2017

dnsmichi commented May 10, 2017

Review

Patch

Tests

Non-persistent

Persistent

Icinga Web 2

dnsmichi commented May 10, 2017

gunnarbeutner commented Feb 8, 2017 •

edited

Loading